Add assert for zero divisor in BigInt divMod to prevent infinite loop#11009
Add assert for zero divisor in BigInt divMod to prevent infinite loop#11009burner wants to merge 1 commit into
Conversation
| @safe pure unittest | ||
| { | ||
| import std.exception : assertThrown; | ||
| assertThrown(BigInt(100) / BigInt(0)); |
There was a problem hiding this comment.
this fails, does assertThrown only catch exceptions?
There was a problem hiding this comment.
It can catch any Throwable, but by default it only catches Exception.
There was a problem hiding this comment.
This is a fragile test either way, it assumes a specific checkaction for asserts. I would not write a unittest for this.
|
Do we really need to crash the program for a divide-by-zero? It's not UB. In fact, it's very well defined behavior. Asserts are for errors in the code itself, not for sanitizing user input. User input crashing the program is an automatic CVE under the DoS category. |
https://dlang.org/spec/expression.html#division "Undefined Behavior: is exhibited if they are encountered during run time." Integer division by zero currently results in a crash with DMD. You may be thinking of floating point division, which has different behaviour.
Wouldn't that mean it should never be possible to trigger an assert in phobos? I think that's a bit unreasonable. |
Walter and I go back and forth on this a few times a year. Integer division behaving differently than float division is a glaring inconsistency in how division is handled. Thank you for giving me this leverage for the next time Walter and I decide to throw down on this topic.
No, it would mean that user inputs do not trigger an assert. An assert is for making sure that the Phobos code itself does nothing wrong. Passing a zero to the divide operation is user input. Yes, it's wrong, but it's not a danger to the system and will not cause UB. Throw the exception so that I can log it without crashing all the other users (and causing actual UB in the process). Crashing works great for single-threaded single-user batch apps. It is global thermonuclear warfare when applied to multi-user multi-threaded apps where crashing the app can interrupt a database update chain and leave user data in a malformed state that requires manual intervention to resolve. It gets worse when you considered that the malformed data can easily trigger another assert (if you use asserts to sanitize data) when loading that malformed data again from the database, causing another crash, which causes more malformed data. Exceptions are for sanitizing user inputs. Asserts are for verifying the correctness of the code. |
I would be quite happy to see floating point division by zero become consistent with integer division by zero.
Unless there is some way we can call phobos functions without user input, then there must be no asserts.
We should not be encouraging the design of such fragile software. This particular problem was solved way back in the 1980s with ACID...
Indeed, but I think I'm going to have to disagree with your definition of "user input". |
Hardly:
The purpose of an assert is to assert that something potentially catastrophic cannot happen, and if it has, then we need to kill the app/thread. Malformed user data cannot be catastrophic to the execution of an application.
False. Not all user data operations can occur inside a single transaction. Not only can you have changes that span multiple transactions, you can have changes that span multiple databases. I ship production code today where a single user data operation spans five transactions. Specifically, I need to commit the transaction to get back the new record ID so that I can add dependent data in a different table. This is a frequent pattern in real-world systems. And not all databases are ACID compliant. The world is more than SQL these days. Working with eventually consistent systems means that user data mismatches MUST be handled without crashing the program.
So we should assert on malformed JSON? If you insist on pursuing an assertion-maximalist position, I can guarantee that D will be thrown into the scrapheap of history immediately by reason of being utterly useless outside an extremely narrow set of software. |
|
All of this philosophical discussion is beside the point. |
|
@pbackus Attribute soup strikes again. These attributes have been nothing but a disaster for API design. I won't block this PR, but I'll put up a work-item for Phobos 3 to remove the nothrow. |
|
Doesn't BigInt have flag bits for invalid states? That would be the path of least resistance here I think. |
Fixes #10784